Add resolved-dependency override mechanism for DR-008 - #278
Conversation
6d78316 to
fbc58a3
Compare
|
The created documentation from the pull request is available at: docu-html |
|
|
||
| # Alias: expose resolve_deps under //scripts/tooling for `bazel run //scripts/tooling:resolve_deps`. | ||
| alias( | ||
| name = "resolve_deps", |
There was a problem hiding this comment.
Why this alias? This is usually for backwards-compatibility but since this is new functionality it should not be necessary.
There was a problem hiding this comment.
The alias under //scripts/tooling provides reachability alongside other entry points. The implementation itself remains in //scripts/known_good since that’s where its models and tests live. The pipeline continues to call //scripts/known_good:resolve_deps directly; the alias simply exposes it without duplicating the target.
| """Unit tests for ResolvedDependencies (DR-008 Option 4 dependency injection). | ||
|
|
||
| Self-contained: builds the resolved set from a temporary known_good.json and | ||
| overwrites a temporary module MODULE.bazel — no cloned repos or Bazel required. |
There was a problem hiding this comment.
I like this self-contained aspect. It allows to use this in an hermetic way.
However, the tradeoff is that we cannot parallelize the followup actions (build, test, docs, etc) via GitHub actions, can we?
There was a problem hiding this comment.
No, they can still run in parallel. Stage 1 resolves once and uploads resolved_versions.json as an artifact; each Stage 2 module job downloads that same artifact and does its own local override + build/test independently, still as a normal parallel matrix. The hermetic is only about the unit tests not needing real clones/bazel to validate scan()/overwrite() — it doesn't force any job serialization in the actual pipeline.
e32b107 to
cff77c1
Compare
OliverHeilwagen
left a comment
There was a problem hiding this comment.
Please fix transitive dependency pinning gap
| import sys | ||
| from pathlib import Path | ||
|
|
||
| _HERE = Path(__file__).resolve().parent |
There was a problem hiding this comment.
Please make use of BUILD_WORKING_DIRECTORY or BUILD_WORKSPACE_DIRECTORY environment variable instead.
There was a problem hiding this comment.
Please use repo_root in case mod_graph is not a absolute path e.g.;
if not mod_graph.is_absolute():
mod_graph = repo_root / mod_graph
Please apply the same pattern to export otherwise the files are only written to runfiles dir.
| if args.resolved_deps: | ||
| resolved = ResolvedDependencies.from_resolved_artifact(args.resolved_deps) | ||
| else: | ||
| resolved = ResolvedDependencies.from_known_good(args.known_good_path) |
There was a problem hiding this comment.
Transitive dependency pinning gap
overwrite() only injects overrides for modules the module-under-test directly declares
via bazel_dep. Transitive dependencies silently fall through to MVS resolution, which may
pick a different version than what ref_int validated against.
Example:
ref_int (resolved: flatbuffers @ 25.12.19)
└── communication (module under test)
└── bazel_dep(name = "score_baselibs") ← override injected ✅
└── flatbuffers ← transitive, NO override injected ❌
MVS picks its own version
Fix: For every module in the resolved set that is not directly declared by the
module-under-test, inject both a bazel_dep stub and the override into the injection block:
# brings the transitive dep into the root graph so the override is valid
bazel_dep(name = "flatbuffers", version = "0.0.0")
git_override(module_name = "flatbuffers", commit = "...", remote = "...")This also means from_known_good is insufficient for the inject mode, as it only carries
first-party score modules and has no transitive registry versions; --resolved-deps (from the
Stage-1 manifest) must be the required source for Stage 2 injection.
In Stage-1 we should also takeover and store the given graph.json as it is needed for Stage-2 to be aware of the necessary transitive overrides for a given MODULE.bazel file. Identification can be done via regex for module(name = "...").
cff77c1 to
fce6a51
Compare
36fe2e9 to
fb20ff4
Compare
eda7170 to
5d9963e
Compare
… markers, simplify imports
…directive inline, add BUILD for bazel run
…wn_good.py, and tooling BUILD format
…ing it, and make resolved_dependencies importable standalone
…nstead of only its declared deps
…label consistency guard
841b5fc to
5b4774b
Compare
|
Hi @OliverHeilwagen, there was a discussion in todays SCORE alignment call and we decided to enable this workflow in parallel to the old integration tests. This will allow us to verify and tackle all the issues with the workflow itself in the transition period as I will let myself merge this PR to enable rebase in #280 and adding this DR8 as parallel workflow. Please do not hesitate to continue reviewing in the part 2 PR or creating the issues directly. Thank you for contributing to the review! |
|
@PiotrKorkus Thanks for the info, i will bring my pending comments over to #280 and continue the review there. |
DR-008: resolved-dependency resolve + override mechanism (PR 1 of 2)
Part of #264. This is the first of two PRs that split the original DR-008 Option 4 change. This PR
adds the mechanism; the follow-up PR wires it into the test workflow.
What this PR does
Adds
ResolvedDependencies— the mechanism DR-008 Option 4 needs to (1) resolve the full integrateddependency set at the reference_integration root and (2) override those resolved versions directly
into an individual module.
MODULE.bazel+bazel_common/*.MODULE.bazel) with the post-MVS registry versions frombazel mod graph --output=json, and serializes the result to a singleresolved_versions.jsonmanifest (
--export/from_mod_graph/to_file). 156 modules today.MODULE.bazelfor every declaredbazel_depandappends a
git_override/single_version_overridefor each one we have a resolved version for,so the module builds/tests against the resolved set (
scan/overwrite). Injection isappend-only and idempotent (marked with begin/end comments), always skips the module-under-test
itself (the root is never overridden), always overwrites any dependency the module already
declares an override for (ref_int's resolved version always wins), and is never committed back to
module sources. A declared dependency with no entry in the resolved set is left to resolve on its
own and logs a warning.
module_patches.pyapplies the module-under-test's own declaredbazel_patchesto its checkout by filesystem path (git apply -p1), before override injection —a patch may add a
bazel_depthat injection must not then duplicate. Distinct from the stripabove: that one drops
bazel_patchesfrom injected dependency overrides (their labels don'tresolve inside another module's checkout); this one is the module-under-test's own patches, which
name real files in ref_int's tree regardless of whose checkout is root. A patch that fails
git apply --checkis a named, loud failure — attributed as a ref_int defect — never a silentskip.
rc_label_consistency.pyis an offline check that ref_int neversets the same build-setting label at two different paths across its own
.bazelrc,ci/stage2/module.bazelrc(added by the follow-up PR), andknown_good.json'sextra_test_configentries — catches a relocated upstream package before it surfaces as anunrelated-looking Stage 2 failure, rather than after.
resolved_pins_report.jsonsidecar recording, per pin,whether ref_int explicitly asserted it or only inherited it from MVS, whether any consumer in
ref_int's own graph wanted a different version, and any override ref_int declares that could not be
carried into the manifest at all (today:
rules_boost'sarchive_override). Nothing here changeswhat gets pinned or fails the export — it makes the existing resolve step's decisions visible.
While testing this against a real checkout it turned up that
rules_ociwas silently falling out ofthe manifest entirely (its
git_overridepinned a mutable tag, which can't become a manifest entry,and nothing warned about it) — fixed by pinning it to a commit instead.
Files
scripts/known_good/resolved_dependencies.pyResolvedDependenciesmechanism (resolve + override + report) + CLIscripts/known_good/module_patches.pybazel_patchesto its checkout ahead of override injectionscripts/known_good/rc_label_consistency.pyscripts/known_good/tests/test_resolved_dependencies.pyscripts/known_good/tests/test_module_patches.pyscripts/known_good/tests/test_rc_label_consistency.pyscripts/known_good/BUILDpy_library :known_good,py_binary :resolve_deps(Bazel-runnable), andscore_py_pytest :known_good_testsso the unit tests run under Bazel.github/workflows/internal_tests.yml:known_good_testsin CIbazel_common/score_images.MODULE.bazelrules_ocipinned by commit instead of tagbazel_common/score_test_artifact_versions.MODULE.bazelgoogletest,rules_rust,score_toolchains_rust); no build change todayMODULE.bazelinclude()s the file aboveMODULE.bazel.lockFollow-up (PR 2)
The two-stage
test_and_docs.yml(Stage 1 resolves + exports the manifest; Stage 2 checks out eachmodule, overrides deps via this mechanism, and runs its UT + coverage; then aggregates), the
module-context
quality_runners.py, the dependency pin bumps, and the associated config all land inthe follow-up PR, which is stacked on this one.